Skip to content

[TextInput] Add __DEV__ around textInput prop check#8499

Closed
dozoisch wants to merge 1 commit into
react:masterfrom
dozoisch:input_dev_prop_check
Closed

[TextInput] Add __DEV__ around textInput prop check#8499
dozoisch wants to merge 1 commit into
react:masterfrom
dozoisch:input_dev_prop_check

Conversation

@dozoisch

Copy link
Copy Markdown
Contributor

Motivation

The prop validation was run on every render, even in prod which seems odd to me. This adds a dev check to make sure prod build skips the validation.

Test plan (required)

There are no changes to the ui.

- the input component is validating props in production on each render which
didn't seem very efficient. This ensures it's only run in development.
@dozoisch dozoisch changed the title Add __DEV__ around proptype check Add __DEV__ around textInput prop check Jun 29, 2016
@ghost

ghost commented Jun 29, 2016

Copy link
Copy Markdown

By analyzing the blame information on this pull request, we identified @JoelMarcey and @Bhullnatik to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 29, 2016
@dozoisch dozoisch changed the title Add __DEV__ around textInput prop check [TextInput] Add __DEV__ around textInput prop check Jun 30, 2016
@javache

javache commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 1, 2016
@ghost

ghost commented Jul 1, 2016

Copy link
Copy Markdown

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 508cc06 Jul 1, 2016
@dozoisch dozoisch deleted the input_dev_prop_check branch July 1, 2016 16:08
@ide

ide commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

Can you change this from an error to a warning? Really don't want different failure modes between dev and prod.

@dozoisch

dozoisch commented Jul 1, 2016

Copy link
Copy Markdown
Contributor Author

@ide if there's a consensus, I don't mind making another PR!

@javache

javache commented Jul 4, 2016

Copy link
Copy Markdown
Contributor

@ide this seems similar to how we do other propType validation, so I think it's fine. https://github.com/facebook/react/blob/master/src/isomorphic/classic/types/ReactPropTypes.js#L149

@ide

ide commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@javache React creates Errors in order to capture the stack trace but it doesn't throw them. It prints a warning during propType validation: https://github.com/facebook/react/blob/95ac239cf3c527b70daeb433b7aeeaf9e41be9e5/src/isomorphic/classic/types/checkReactTypeSpec.js#L84

@dozoisch

dozoisch commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

@ide so should we change it so that it only prints it instead of throwing?

@ide

ide commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

Yea I think we want to follow what React does with warning.

@dozoisch

dozoisch commented Jul 8, 2016

Copy link
Copy Markdown
Contributor Author

@ide dozoisch@534d6ce would that be ok with you? If yes, I'll create a pr. Sorry for not responding, never saw your response 3 days ago!

dozoisch added a commit to dozoisch/react-native that referenced this pull request Jul 13, 2016
- the goal is to have the same failure mode in prod and dev. See conv in react#8499
ghost pushed a commit that referenced this pull request Jul 13, 2016
Summary:
Follow up of #8499! Open to discussion

Motivation:

See #8499 (comment)

Goal is to have the same failure mode in dev and prod.

/cc ide
Closes #8757

Differential Revision: D3558991

fbshipit-source-id: c7d133f958e67ab23da486b1ffcb8f9963509b79
tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
Summary:
**Motivation**

The prop validation was run on every render, even in prod which seems odd to me. This adds a dev check to make sure prod build skips the validation.

**Test plan (required)**

There are no changes to the ui.
Closes react/react-native#8499

Differential Revision: D3508805

Pulled By: javache

fbshipit-source-id: 68c6a1224e33997f9df93481426daff790ef5bcd
tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
Summary:
Follow up of react/react-native#8499! Open to discussion

Motivation:

See react/react-native#8499 (comment)

Goal is to have the same failure mode in dev and prod.

/cc ide
Closes react/react-native#8757

Differential Revision: D3558991

fbshipit-source-id: c7d133f958e67ab23da486b1ffcb8f9963509b79
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants